[glsl-in] Don't reinterpret function arguments twice in the normal case #6604
Conversation
We already lowered the function expression if it's a normal in argument; no need to do it again. This eliminates an unused variable and expression. With chained function calls, this could lead to a lot of waste. There's still an extra unused expression in the case of an out/inout argument; ideally we'd remove these expressions, but it might be tricky. Fixes gfx-rs#6602
There was a problem hiding this comment.
The code changes look good to me, but I think it should have a test that clearly shows what's being fixed. The effects I saw on the snapshot output files were:
-
In the (handful of) GLSL snapshot inputs that do use
outorinoutparameters, there were no changes. -
In those that do not it cleaned up some unused temporaries, but I didn't notice any code going from incorrect to correct.
So unless I missed it, that means we don't currently have any snapshot inputs that demonstrate why this fix is necessary.
|
Yeah, the code generated was technically fine before, but it was a lot of code to chew through, and with a lot of functions, you could easily get a lot of temporaries emitted when none were necessary. In particular, it generated code that caused Chrome to take 20s or so to compile the pipeline. Before this change, I had a shader that emitted a lot of unused temporaries: https://gist.github.com/magcius/95298da1c344376cd9090c12a44bffdb#file-test-wgsl-L542-L618 With this change, that now gets reduced to this, which is still a lot of code, but way less before, and now at least every temporary is used: let _e85 = global_1.u_LightParams[1];
let _e88 = v_Position;
t_LightDelta = (_e85.Position.xyz - _e88.xyz);
let _e91 = t_LightDelta;
let _e92 = t_LightDelta;
t_LightDeltaDist2_ = dot(_e91, _e92);
let _e94 = t_LightDeltaDist2_;
t_LightDeltaDist = sqrt(_e94);
let _e96 = t_LightDelta;
let _e97 = t_LightDeltaDist;
t_LightDeltaDir = (_e96 / vec3(_e97));
let _e104 = global_1.u_LightParams[1];
let _e108 = t_LightDeltaDir;
let _e111 = global_1.u_LightParams[1];
let _e116 = ApplyAttenuation(_e104.CosAtten.xyz, max(0f, dot(_e108, _e111.Direction.xyz)));
let _e120 = global_1.u_LightParams[1];
let _e125 = t_LightDeltaDist;
let _e126 = t_LightDeltaDist2_;
t_Attenuation = max(0f, (max(0f, _e116) / dot(normalize(_e120.DistAtten.xyz), vec3<f32>(1f, _e125, _e126))));
let _e131 = t_LightAccum;
let _e132 = t_Normal;
let _e133 = t_LightDeltaDir;
let _e137 = t_Attenuation;
let _e141 = global_1.u_LightParams[1];
t_LightAccum = (_e131 + ((max(dot(_e132, _e133), 0f) * _e137) * _e141.Color));
Yes, there aren't any changes for inout/out, those will still generate unused temporaries; the function code first generates an expression for the parameter assuming it's in an RHS position, and it doesn't look easy to fix that. So it's not a full fix, yet, but inout/out are much more rare by comparison. |
Connections
Fixes #6602
Description
We already lowered the function expression if it's a normal in argument; no need to do it again. This eliminates an unused variable and expression. With chained function calls, this could lead to a lot of waste.
There's still an extra unused expression in the case of an out/inout argument; ideally we'd remove these expressions, but it might be tricky.
Testing
See shader changes.
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.